Protect exactly what the last scan saw when materializing - #188
Merged
Conversation
A local rename can end with both the old and the new path present on every peer. The sync pass materializes with a disk view it captured before the peer step. A file that an inbound session materialized during the peer step is missing from that view, so a rename that lands between the post-peer scan and its materialization looks like a file this machine never had, and the old path is written back. This adds test-only seams between each scan and the materialization that follows it, and five tests that pin events to those seams: - a rename at the post-peer seam splits the file (fails today), - a file created and removed inside one inbound session comes back (fails today), - a file created and removed around the pre-peer scan comes back (fails today), - the same rename one seam earlier is recorded by the scan (passes), - a remote-only path adopted during the peer step is still written (passes). No behavior changes. Refs #175.
A local rename could leave both the old and the new path present on every peer. The sync pass captured a disk view before its peer step and materialized with that view after it. An inbound session that ran during the peer step had already written the peer's file to disk, so the view lacked it. A rename landing between the post-peer scan and its materialization then read as a file this machine never had, and the old path was written back. The delete was never recorded. Every materialization now takes its protected view for itself, under the caller's operation guard, at the start of each attempt. That view is what the scan immediately before it published. The function no longer accepts a caller's view, so no caller can hand it a stale one. The same rule stops a file created and removed inside one pass or one inbound session from coming back. The seam hooks and the hook slot are compiled only in test builds. A non-test `cargo check --lib` passes, and the release binary holds no seam symbol while it holds 61 symbols for the surrounding functions. Fixes #175.
Collaborator
Author
|
Integration samples for the record. These are reported, not used as proof. The test
Both series ran serially on one macOS machine with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A local rename could leave both the old and the new path present on every peer. Fixes #175.
What went wrong
A sync pass has two guarded halves around an unguarded peer step. The first half scanned, materialized, and captured a disk view. The second half scanned again and materialized with that older view.
During the peer step the guard is free. An inbound session from the peer can run then, and it materializes the peer's file onto local disk. The older view does not contain that file. If a rename of that file lands between the second scan and its materialization, the materialization sees a Present manifest entry, a missing file, and no record that the file was ever here. That combination means "remote-only file, write it", so the old path was written back. The next scan found both paths present and recorded the new one. The delete was never recorded, so it could never propagate.
The daemon's sync reload runs a pass for every entry while the entry loop may already be in one, so two passes on one entry can interleave through the guard. That is what produced the overlap in the integration test.
The same shape existed in the first half of a pass and in the completion of an inbound session: a file created inside the window and removed between the scan and its materialization came back.
The rule
Every materialization protects exactly what the scan immediately before it saw, under the same hold of the operation guard.
materialize_entry_statenow takes that view for itself at the start of each attempt. Itsprotectedparameter is removed, so no caller can pass a stale view again. The baselines the callers carried across the peer step and the wire session now decide only whether there is anything to persist.Proof
Five new tests pin events to test-only seams between each scan and the materialization that follows it. Three reproduce the defect and two are controls.
The first test asserts, before it looks at the outcome, that the seam fired while the file was on disk and Present in the manifest. The last control fails if a fix protects every manifest path instead of every scanned path, which would stop remote-only files from being written.
The full library suite passed 538 tests with 5 ignored on the final head, 23:01:24Z to 23:01:55Z.
The seams do not exist in production builds
The seam enum, the hook slot,
set_seam_hook,at_seam, and every call toat_seamare undercfg(test). A production reference to any of them cannot compile, and a non-testcargo check --libpasses. The release binary built from this branch holds 0 symbols matching the seam names against 61 symbols for the surrounding functions and 62,805 symbols in total.Out of scope
A remote edit of the source at a higher version, concurrent with the local rename, brings the edited source back beside the new path. Present wins over a tombstone at an equal version and a higher version wins outright. That is the designed newer-wins rule for a real concurrent edit and rename, not this defect.
A crash between the scan and the persist leaves the durable state at the pre-rename version with the source still in the observed receipts. The first scan after restart finds the source absent and observed, records the tombstone, and records the destination as new. Nothing about this change alters persistence.